Skip private/internal IPs when detecting the client IP from headers#4329
Skip private/internal IPs when detecting the client IP from headers#4329kriscarle wants to merge 1 commit into
Conversation
When a request passes through internal proxies or load balancers (e.g. Nomad/fabio behind a cloud load balancer), a header such as `x-real-ip` or `forwarded` can carry an internal address (10.x, 172.16-31.x, 192.168.x, loopback, link-local, CGNAT, IPv6 ULA) while the real client IP is present in another header like `x-forwarded-for`. Because the previous logic picked the first header that was merely present, it could return the internal IP. Iterate the candidate headers in priority order, normalize each value, and skip private/internal addresses, falling through to the next header that carries a public client IP. If every candidate is private, the first one is still returned as a last resort so behaviour is unchanged for local setups. Also route the `CLIENT_IP_HEADER` path through the same extraction so a custom header pointing at a multi-IP `x-forwarded-for` list resolves the first entry instead of the raw list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@kriscarle is attempting to deploy a commit to the Umami Software Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes
Confidence Score: 4/5Safe to merge with low risk; the change only affects IP resolution ordering and is backward-compatible for single-proxy deployments. The core loop logic is correct and well-tested for the described multi-proxy scenario. Two narrow gaps exist: isPrivateIp fails silently when the Forwarded header delivers a bracketed IPv6 (e.g. [::1]), causing a loopback address to bypass the private check; and the CLIENT_IP_HEADER early-return path intentionally skips the private-IP filter but has no comment documenting this. Neither gap affects the common case. src/lib/ip.ts — specifically the isPrivateIp interaction with extractIp for the forwarded header. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request arrives at getIpAddress] --> B{CLIENT_IP_HEADER set?}
B -- Yes --> C[Get header value]
C --> D{Value present?}
D -- Yes --> E[extractIp: handle XFF/Forwarded/raw]
E --> F[resolveIp]
F --> G[Return IP - no private check]
D -- No --> H
B -- No --> H[Loop IP_ADDRESS_HEADERS in priority order]
H --> I{Header value present?}
I -- No --> J[Next header]
J --> I
I -- Yes --> K[extractIp]
K --> L[resolveIp]
L --> M{ip truthy?}
M -- No --> J
M -- Yes --> N{isPrivateIp?}
N -- Yes --> O{fallback set?}
O -- No --> P[fallback = ip]
O -- Yes --> J
P --> J
N -- No --> Q[Return public IP]
J -- exhausted --> R[Return fallback or undefined]
style G fill:#f9c,stroke:#c66
style Q fill:#cfc,stroke:#6c6
style R fill:#ffc,stroke:#cc6
Reviews (1): Last reviewed commit: "Skip private/internal IPs when detecting..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (header === 'forwarded') { | ||
| const match = value.match(/for=(\[?[0-9a-fA-F:.]+]?)/); | ||
|
|
||
| return match ? match[1] : undefined; |
There was a problem hiding this comment.
isPrivateIp bypassed for bracketed IPv6 in Forwarded header
The extractIp regex /for=(\[?[0-9a-fA-F:.]+]?)/ can capture the brackets as part of the match, e.g. Forwarded: for=[::1] yields [::1] as the captured group. ipaddr.parse('[::1]') throws (brackets are not valid in the parser's notation), so isPrivateIp silently returns false, and the IPv6 loopback [::1] bypasses the private-IP guard and is returned as the client address. Stripping brackets before calling isPrivateIp (and before the resolveIp call) would close this gap — the plain form ::1 parses correctly and .range() returns 'loopback'.
| if (customHeader) { | ||
| const value = headers.get(customHeader); | ||
|
|
||
| if (value) { | ||
| return resolveIp(extractIp(customHeader, value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
CLIENT_IP_HEADER path skips private-IP check
When CLIENT_IP_HEADER is set to a header that contains a private address (e.g. CLIENT_IP_HEADER=x-real-ip and x-real-ip: 10.0.0.8), the code returns that private IP immediately without applying the isPrivateIp filter or falling through to other headers. This is a deliberate design choice (the user explicitly chose that header), but it means the custom-header path can still return internal addresses that break geolocation. A comment noting this intentional behavior would help future readers understand why the private-IP logic is omitted here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
When a request reaches umami through more than one proxy/load balancer,
getIpAddress()could return an internal address (e.g.10.0.0.8) instead of the real client IP, which then breaks geolocation and IP-based filtering.The previous logic picked the first candidate header that was merely present (
IP_ADDRESS_HEADERS.find(...)) and never checked whether the value was an internal/proxy address. This change iterates the candidate headers in priority order, normalizes each value, and skips private/internal addresses, falling through to the first header that carries a public client IP.Root cause
In a multi-proxy chain (in our case
client → cloud load balancer → fabio → fabio → umami), only the leftmost entry ofX-Forwarded-Forreliably holds the original client. Other headers carry the previous hop, not the originator:X-Forwarded-Foris append-only. Per MDN, "the rightmost IP address is the IP address of the most recent proxy and the leftmost IP address is the address of the originating client." So the internal proxy hops legitimately appear in the list alongside the client.The
Forwardedheader'sfor=is the immediate peer. RFC 7239 definesfor=as "the node making the request to the proxy" — i.e. the previous hop. Proxies such as fabio set bothForwarded: for=andX-Real-Ipfrom the incoming connection'sRemoteAddr:By the time the request reaches such a proxy,
RemoteAddris an internal node (e.g.10.0.0.8), so that internal IP ends up inForwarded(always) and inX-Real-Ip(whenever an upstream hasn't already stamped the real client).Because
forwarded/x-real-ipcan therefore legitimately contain an internal address, selecting the first present header without inspecting its value yields the wrong IP. This is a generic multi-proxy artifact rather than a single-proxy misconfiguration.The fix
IP_ADDRESS_HEADERSin order; for each, extract the candidate IP (first entry forx-forwarded-for,for=forforwarded),resolveIp()it, and skip it if it is private/internal (RFC1918, loopback, link-local, CGNAT, IPv6 unique-local — detected viaipaddr.range()).CLIENT_IP_HEADERpath now goes through the same extraction, so a custom header pointed at a multi-IPx-forwarded-forlist resolves the first entry instead of the raw comma-separated string.Header priority order is unchanged — the fix is purely additive (skip-private), so setups whose first header is already public behave exactly as before.
Tests
Added two cases to
src/lib/detect.test.ts:x-real-ipwith a publicx-forwarded-forresolves to the public client IP;CLIENT_IP_HEADER=x-forwarded-forwith a multi-IP list extracts the first IP.Existing
getIpAddresstests continue to pass.References
proxy/http_headers.goNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.